🚀 Package Preview Available!

Install this PR's preview build with npm:

npm i @base44-preview/cli@0.0.1-pr.18.d6d2f8e

Prefer not to change any import paths? Install using npm alias so your code still imports base44:

npm i "base44@npm:@base44-preview/cli@0.0.1-pr.18.d6d2f8e"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "base44": "npm:@base44-preview/cli@0.0.1-pr.18.d6d2f8e"
  }
}

• 📦 Preview Package: @base44-preview/cli@0.0.1-pr.18.d6d2f8e
• 🔗 View this commit on GitHub

_{Preview published to npm registry — try new features instantly!}

Pull Request Review: Refactor circular deps, api clients issues, configs

This PR makes significant architectural improvements to resolve circular dependencies and improve the separation of concerns. Overall, this is a well-structured refactoring that enhances code organization and maintainability.

✅ Strengths

1. Excellent Separation of Concerns

• New src/core/clients/ directory: Separates OAuth client (unauthenticated) from Base44 client (authenticated) - this is a clean architectural decision
• New src/core/consts.ts: Breaking out constants eliminates circular dependency issues
• New src/core/project/types.ts and src/core/resources/types.ts: Proper separation of type definitions from implementation

2. Improved API Client Design

The base44Client implementation (src/core/clients/base44-client.ts) demonstrates excellent patterns:

• ✅ Proactive token refresh before requests (lines 65-72)
• ✅ Reactive retry with WeakSet to prevent infinite loops (lines 16, 32-34)
• ✅ Clear separation of concerns with hooks
• ✅ Good error handling with fallback behavior

3. Better Error Messages

The getAppClient() function now provides actionable error messages when BASE44_CLIENT_ID is missing (lines 96-99), guiding users to set it in .env.local.

4. Enhanced Developer Experience

• runCommand() utility now has options for fullBanner - nice UX improvement
• Added JSDoc comments throughout for better IDE support
• runTask() now includes example usage in documentation

5. Documentation Updates

Both AGENTS.md and README.md were thoroughly updated to reflect the new architecture, including:

• Clear diagrams of the new folder structure
• API client usage examples
• Updated rules (e.g., "consts.ts has no imports")

⚠️ Issues & Concerns

1. Missing Type Export in project/types.ts (line 5)

interface ProjectWithPaths extends ProjectConfig {

This should be export interface ProjectWithPaths to make it available for external use. Currently it's only used internally in the file, but if it's meant to be part of the public API from types.ts, it should be exported.

2. Potential Race Condition in Token Refresh

In src/core/clients/base44-client.ts (lines 60-78), there's a subtle issue:

beforeRequest: [
  async (request) => {
    try {
      const auth = await readAuth();
      
      // Proactively refresh if token is expired
      if (isTokenExpired(auth)) {
        const newAccessToken = await refreshAndSaveTokens();
        if (newAccessToken) {
          request.headers.set("Authorization", `Bearer ${newAccessToken}`);
          return;
        }
      }
      
      request.headers.set("Authorization", `Bearer ${auth.accessToken}`);

Issue: If multiple requests fire simultaneously before a token refresh completes, they might all read the old expired token and attempt to use it. While refreshAndSaveTokens() has a lock (line 86), the readAuth() call happens before checking the lock.

Recommendation: Consider reading auth once after the potential refresh:

const auth = await readAuth();
if (isTokenExpired(auth)) {
  const newAccessToken = await refreshAndSaveTokens();
  if (newAccessToken) {
    request.headers.set("Authorization", `Bearer ${newAccessToken}`);
    return;
  }
}
request.headers.set("Authorization", `Bearer ${auth.accessToken}`);

Actually, looking closer, the current implementation seems fine because refreshAndSaveTokens() has a lock. However, you might want to re-read auth after refresh to ensure consistency.

3. Error Handling Inconsistency in auth/config.ts

Lines 37-42 removed specific error handling that distinguished between different failure modes:

-    if (error instanceof Error && error.message.includes("Authentication")) {
-      throw error;
-    }
-    if (error instanceof Error && error.message.includes("File not found")) {
-      throw new Error("Authentication file not found. Please login first.");
-    }

Why this matters: The generic error message "Failed to read authentication file" is less helpful than "Please login first" when the auth file doesn't exist. Users will see a confusing error instead of clear guidance.

Recommendation: Restore the specific error handling for file not found scenarios.

4. Missing Validation in pushEntities()

In src/core/resources/entity/api.ts (lines 21-23):

const errorJson: { message: string } = await response.json();

This assumes the error response always has a message field. If the API returns a different error format, this could throw an unhandled error.

Recommendation: Use Zod schema validation or add a fallback:

const errorJson = await response.json();
const message = typeof errorJson?.message === 'string' 
  ? errorJson.message 
  : 'Unknown error';

5. Comment Clarity Issue

Line 7 in src/core/auth/config.ts:

// Buffer time before expiration to trigger proactive refresh (60 seconds)
const TOKEN_REFRESH_BUFFER_MS = 60 * 1000;

The comment is good, but the removal of other comments (lines that were deleted) made some functions less clear. For example, refreshAndSaveTokens() lost its documentation.

🔒 Security Considerations

✅ Good Practices

1. Token refresh buffer of 60 seconds is reasonable
2. Proper cleanup on refresh failure (deletes auth file)
3. WeakSet prevents retry loops

⚠️ Minor Concerns

1. Silent auth failures: In base44Client (lines 75-77), auth errors are silently caught. While this allows unauthenticated requests to proceed, it could mask legitimate auth problems. Consider logging a debug message.

🚀 Performance Considerations

✅ Optimizations

1. Parallel resource loading: readProjectConfig() uses Promise.all() (line 88) - excellent
2. Request lock for token refresh: Prevents duplicate refresh requests
3. WeakSet for retry tracking: Memory-efficient, automatic cleanup

ℹ️ Minor Notes

• The retriedRequests WeakSet (line 16) is module-level and will persist for the CLI's lifetime. This is fine for a CLI tool but could be an issue in a long-running server. Not a concern here.

📝 Test Coverage

Missing: The PR doesn't include tests for the new client architecture. Consider adding tests for:

1. Token refresh flow (proactive and reactive)
2. Retry logic with WeakSet
3. Error scenarios (expired tokens, network failures)
4. getAppClient() with missing BASE44_CLIENT_ID

🎯 Code Quality

Style & Consistency

• ✅ Consistent use of JSDoc comments
• ✅ Proper TypeScript typing throughout
• ✅ ES modules with .js extensions (following project conventions)
• ✅ Good use of Zod validation

Naming

• ✅ Clear, descriptive function names
• ✅ Consistent naming patterns (read*, get*, etc.)

📋 Recommendations Summary

High Priority

1. ❗ Restore specific error handling for "file not found" in readAuth()
2. ❗ Add validation/fallback for error response parsing in pushEntities()

Medium Priority

3. Consider exporting ProjectWithPaths from types.ts if it's meant to be public
4. Add debug logging for silent auth failures in base44Client

Low Priority

5. Add tests for new client architecture
6. Consider re-reading auth after refresh for consistency

✨ Overall Assessment

Verdict: ✅ Approve with minor suggestions

This is a solid refactoring that significantly improves the codebase architecture. The circular dependency issues are resolved, the client separation is clean, and the documentation is excellent. The issues identified are mostly minor and don't block merging, but addressing the error handling regression would improve user experience.

Great work on:

• Clean architectural separation
• Thoughtful token refresh implementation
• Comprehensive documentation updates
• Consistent coding patterns

The changes align well with the project's conventions documented in AGENTS.md.

Pull Request Review: Refactor - Solve Circular Dependencies, API Clients Issues, Configs

Overview

This PR successfully addresses architectural issues in the codebase by resolving circular dependencies, reorganizing API clients, and consolidating configuration management. The changes improve code organization and maintainability.

✅ Strengths

1. Excellent Circular Dependency Resolution

• Moving constants to consts.ts: Creating a dependency-free constants file is a solid pattern that prevents circular imports.
• Separating OAuth and authenticated clients: The split into oauth-client.ts and base44-client.ts is architecturally sound.

2. Improved Code Organization

• New src/core/clients/ directory: Consolidating HTTP clients into a dedicated directory improves discoverability.
• Type consolidation: Moving types to dedicated files reduces coupling and improves reusability.

3. Enhanced Developer Experience

• Comprehensive documentation updates: Both README.md and AGENTS.md received substantial improvements.
• Better error messages: getAppClient() now provides clear guidance when BASE44_CLIENT_ID is missing.

🐛 Potential Issues

1. Token Refresh Race Condition - CRITICAL

Location: src/core/clients/base44-client.ts:15-48

The retriedRequests WeakSet has a race condition. If two requests fail with 401 simultaneously, both will pass the has() check and trigger concurrent refreshes before either adds to the WeakSet.

Recommendation: Add the request to the WeakSet immediately after the first check.

2. Silent Auth Failures

Location: src/core/clients/base44-client.ts:75-77

All errors are silently swallowed in the beforeRequest hook, including corrupted auth files and validation errors.

Recommendation: Differentiate between not logged in vs auth system error for better user feedback.

3. Missing Error Context

Location: src/core/auth/config.ts:37-43

The readAuth() error handling masks original error types by wrapping everything.

🔒 Security Considerations

Good Practices:

• Token refresh buffer prevents race conditions
• Automatic token cleanup on refresh failure
• Environment variable validation

Minor Concerns:

• No rate limiting on token refresh
• Stack traces might expose filesystem structure

📊 Test Coverage

Missing tests for:

• Token refresh logic
• getAppClient() error behavior
• OAuth client separation
• refreshAndSaveTokens() locking
• runCommand() error handling

Recommendation: Add unit tests for the new client architecture.

🎬 Summary

This is a well-executed refactor that significantly improves the codebase architecture.

Blockers (Must Fix)

1. Token refresh race condition in retriedRequests WeakSet usage

Recommended Before Merge

2. Improve error handling in auth client hook
3. Add unit tests for new client architecture
4. Preserve error context in readAuth()

Nice to Have

5. Add rate limiting/backoff for token refresh
6. Extract TOKEN_REFRESH_BUFFER_MS to consts.ts
7. Document token refresh mechanism

This PR is nearly ready for merge pending the critical race condition fix. Great work!